chore: add e2e tests for custom linting rules#9989
Conversation
✅ Circular References ReportGenerated at: 2026-06-05T22:41:29.247Z Summary
Click to view all circular references in PR (8)Click to view all circular references in base branch (8)Analysis✅ No Change: This PR does not introduce or remove any circular references. This report was generated automatically by comparing against the |
399fcdb to
74d3d0a
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new smoke test suite that exercises uploading/removing custom Spectral rulesets across persistence boundaries (app relaunch), cloud sync round-trips (including a 2-user scenario), and git-sync filesystem mirroring. To support this, the smoke-test mock APIs and Playwright harness gain a few capabilities (multi-user org data, decrypting pushed blobs, and a reusable relaunchable Electron launcher).
Changes:
- Add a comprehensive smoke test suite for custom Spectral lint rules across local, cloud-sync, and git-sync scenarios.
- Add a Playwright/Electron test hook for
showOpenDialog, plus Playwright harness support for process-level relaunch and multi-app teardown. - Extend smoke-test mock servers to support a second “User B” identity and decrypt pushed cloud-sync blobs for round-trip validation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/insomnia/src/main/ipc/electron.ts | Adds a Playwright-only showOpenDialog hook to return queued dialog results instead of opening a native dialog. |
| packages/insomnia-smoke-test/tests/smoke/custom-lint-rules.test.ts | New E2E suite covering upload/remove/persist + cloud sync + git-sync parity for custom Spectral rules. |
| packages/insomnia-smoke-test/server/insomnia-api.ts | Adds mock org/project data for a second user used in multi-user cloud-sync tests. |
| packages/insomnia-smoke-test/server/cloud-sync-api.ts | Replaces node-forge AES-GCM usage with Node crypto, adds design-project data, multi-user team member keys toggle, and decrypts pushed blobs. |
| packages/insomnia-smoke-test/playwright/test.ts | Refactors Electron launch into a shared helper and ensures all launched Electron apps are closed during teardown (supports relaunch). |
| packages/insomnia-smoke-test/playwright/pages/insomnia-app.ts | Enables InsomniaApp.relaunch() (process-level) and adds queueOpenDialogResponse() for the dialog hook. |
| packages/insomnia-smoke-test/playwright/launch.ts | New shared Electron launcher + liveApps tracking for reliable teardown. |
| packages/insomnia-smoke-test/fixtures/files/custom.spectral.yaml | Adds a Spectral ruleset fixture used by the new smoke tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
51157c0 to
2feb9da
Compare
2feb9da to
49d6987
Compare
fiosman
left a comment
There was a problem hiding this comment.
Thanks for your work on this! Appreciate that you were trying to cover as much as possible. I left some comments that we should consider.
Also, we should consider whether we care about testing persistence on app reloads etc (e.g. relaunch function you added). Sometimes it's not worth it if it adds complexity/maintenance efforts to an already flaky/large e2e test suite. In my opinion, end to end tests are supposed to be limited to critical user paths (in this case; on a cloud sync project, one test that uploads a custom ruleset, spec is automatically linted, diff shows up in commit modal, and I am able to push the changes). Unit tests and/or component tests, which run much faster, can cover edge cases. Lmk your thoughts! Great work.
| extends: [spectral:oas] | ||
| rules: | ||
| require-x-smoke-test-marker: | ||
| description: info object must contain a custom x-smoke-test-marker field | ||
| message: "{{description}}" | ||
| severity: error | ||
| given: $.info | ||
| then: | ||
| field: x-smoke-test-marker | ||
| function: truthy |
There was a problem hiding this comment.
This is great, should we also include a fixture with disallowed top level keys (e.g. custom functions). In the smoke test we can make assertion that an error pops up when linting the spec. We just need 1 test to check the modal pops up etc.
This is totally OK to leave out; technically it's an edge case that I think I am already covering in a unit test for a validation function I wrote.
There was a problem hiding this comment.
Valid, I'll add it !
| await expect.soft(page.getByRole('button', { name: 'View selected ruleset content' })).toBeVisible({ | ||
| timeout: 15_000, | ||
| }); |
There was a problem hiding this comment.
Do you think it's worth opening up the modal and verifying the content matches what the ruleset content is?
There was a problem hiding this comment.
Actually it might be, I'll add something in
| // fresh machine, pulls the project, and verifies the ruleset survived the | ||
| // sync handoff. | ||
| // ------------------------------------------------------------------------- | ||
| test.describe.serial('different users, same project', () => { |
There was a problem hiding this comment.
Should we be using serial? If I were to add a .only to any of the tests below they would fail because they rely on state from previous tests? We don't currently use this anywhere in our codebase.
There was a problem hiding this comment.
I think you're right, let me merge the serial pairs into a single test, it's only one scenario anyway. Actually, on also checking playwright's docs, serial is discouraged for use, so good call here!
| if (process.env.PLAYWRIGHT === 'true') { | ||
| const queue = globalThis.__PLAYWRIGHT_OPEN_DIALOG_QUEUE__; | ||
| if (queue && queue.length > 0) { | ||
| return queue.shift(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we utilize existing mockOpenDialogForDirectory? It seems to mock behaviour for native file picker which I think is what you are trying to do this here.
There was a problem hiding this comment.
Good catch on the inconsistency. The reason I didn't reuse it is that mockOpenDialogForDirectory works by permanently replacing the ipcMain handler (removeHandler + handle), so after it fires the real dialog is gone for the rest of the process. The queue approach is just a list that gets shifted on each call and falls through to the real dialog when empty, which felt safer for tests that might open multiple dialogs. Happy to revisit if I'm mistaken or we just don't need the queue
1ac1ad4 to
7c9346d
Compare
7c9346d to
fa3bdc6
Compare
No description provided.